Skip to content

feat: isdlock support without quorum rotation (regtest only) - #5259

Merged
PastaPastaPasta merged 3 commits into
dashpay:developfrom
ogabrielides:regtest_isdlocks
Mar 20, 2023
Merged

feat: isdlock support without quorum rotation (regtest only)#5259
PastaPastaPasta merged 3 commits into
dashpay:developfrom
ogabrielides:regtest_isdlocks

Conversation

@ogabrielides

@ogabrielides ogabrielides commented Mar 16, 2023

Copy link
Copy Markdown

Issue being fixed or feature implemented

In #5229, we disabled rotation when running a regtest with 3 nodes.
Instantsend were non-deterministic islocks.
This PR aims to make them deterministic for a 3-nodes regtest network (Platform dev environment).

What was done?

Added the possibility to overwrite llmqinstantsend and llmqinstantsenddip0024.
By starting a regtest with:
-llmqinstantsend=llmq_test
-llmqinstantsenddip0024=llmq_test_instantsend

Instantsend locks will be deterministic without running quorum rotation.

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@ogabrielides ogabrielides added this to the 20 milestone Mar 16, 2023
Comment thread src/chainparams.cpp Outdated
Comment thread src/chainparams.cpp Outdated
Comment thread src/chainparams.cpp
std::string strLLMQType = gArgs.GetArg("-llmqinstantsend", std::string(llmq_params_opt->name));

Consensus::LLMQType llmqType = Consensus::LLMQType::LLMQ_NONE;
for (const auto& params : consensus.llmqs) {

@knst knst Mar 16, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it seems as this loop already implemented several times:

         if (auto optLLMQParams = ranges::find_if_opt(Params().GetConsensus().llmqs, 
                                                     [&strLLMQType](const auto& params){return params.name == strLLMQType;})) {

in src/llmq/utils.cpp and src/chainparams.cpp:

    Consensus::LLMQType llmqType = Consensus::LLMQType::LLMQ_NONE;
    for (const auto& params : consensus.llmqs) {
        if (params.name == strLLMQType) {
            llmqType = params.type;
        }
    }

May be it's time to refactor to a new method:

    const Consensus::LLMQParams& GetLLMQ(const std::string& llmqTypeName) const;

?

@ogabrielides

ogabrielides commented Mar 17, 2023

Copy link
Copy Markdown
Author

@knst All your suggestions are valid.
Similar refactoring can be made in UpdateDevnetLLMQInstantSendFromArgs, UpdateDevnetLLMQInstantSendDIP0024FromArgs and since those are irrelevant to this PR..
Please feel free to open a new PR refactoring all the cases (once this gets merged) :)

@ogabrielides

Copy link
Copy Markdown
Author

@PastaPastaPasta @UdjinM6 Moved this to v19 as requested by @markin-io.

UdjinM6
UdjinM6 previously approved these changes Mar 20, 2023

@UdjinM6 UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for squash merge (built locally)

@PastaPastaPasta
PastaPastaPasta merged commit 444bc61 into dashpay:develop Mar 20, 2023
@ogabrielides
ogabrielides deleted the regtest_isdlocks branch March 20, 2023 15:59
PastaPastaPasta pushed a commit that referenced this pull request Oct 30, 2023
…enddip0024` (#5654)

## Issue being fixed or feature implemented
This reverts #5636 and introduces 2 similar cmd-line/config params which
are made specifically for regtest. Turned out Platform guys actually
still need smth like that for local testing #5259.

## What was done?
pls see individual commits

## How Has This Been Tested?
run tests but we don't really have(/need?) tests for this.

## Breaking Changes
n/a

## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_

cc @shumkov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants